[10.x] Refactored LazyCollection::take() to save memory #48382

fuwasegu · 2023-09-13T14:18:18Z

Introduction

LazyCollection utilizes Generators to process iterators in a memory-efficient manner.

LazyCollection implements the same interface as Collection, so most operations you can do with a Collection can also be done with a LazyCollection. However, there are methods where it's challenging to implement the functionality without using extra memory (e.g., sorting).

For such methods, we use passthru() to convert LazyCollection to Collection and delegate the task. Be cautious when using collect(), as this will load all the data generated by the Generator into memory, defeating the purpose of using LazyCollection.

Ideally, one would want to avoid using passthru() in LazyCollection.

Main Topic

Originally, the take() method of LazyCollection was implemented like this:

public function take($limit)
{
    if ($limit < 0) {
        return $this->passthru('take', func_get_args());
    }

    return new static(function () use ($limit) {
        $iterator = $this->getIterator();
        while ($limit--) {
            if (! $iterator->valid()) {
                break;
            }
            yield $iterator->key() => $iterator->current();
            if ($limit) {
                $iterator->next();
            }
        }
    });
}

When $limit is a positive value, it uses a Generator function and remains memory-efficient. However, when $limit is a negative value, it uses passthru().

Even for negative $limit values, which take values from the end of the Collection, you can implement it using a Generator function.

Implementation

I've modified the behavior when $limit is negative as follows:

if ($limit < 0) {
            return new static(function () use ($limit) {
                $limit = abs($limit);
                $ringBuffer = [];
                $position = 0;

                foreach ($this as $key => $value) {
                    $ringBuffer[$position] = [$key, $value];
                    $position = ($position + 1) % $limit;
                }

                for ($i = 0; $i < $limit; $i++) {
                    if (isset($ringBuffer[($position + $i) % $limit])) {
                        [$key, $value] = $ringBuffer[($position + $i) % $limit];
                        yield $key => $value;
                    }
                }
            });
        }

Although this implementation still loads $limit absolute value elements into memory, it's more memory-efficient than using passthru() to extract all elements.

P.S.
I first used array_shift to realize queue, but after review, I modified it to use ring buffer.

About Tests

This is a refactoring, so I haven't modified any tests. The fact that tests didn't require changes proves that the refactoring was done correctly without affecting the outcomes.

taylorotwell · 2023-09-13T14:37:13Z

Marking as draft pending @JosephSilber's review.

JosephSilber

Even for negative $limit values, which take values from the end of the Collection, you can implement it using a Generator function.

Absolutely. When I originally wrote this, I didn't implement the negative take lazily since it's quite complex to implement, and has a relatively obscure use-case¹.

But if you want to go through the process of implementing it... 💪

The main issue with this implementation is that array_shift is a relatively expensive operation. It has to rekey the whole array and move up every item. Depending on the size of $limit, this could be very costly. Running this in a tight loop is a no-go.

The correct way to do it is with a FixedSizeQueue data structure (usually backed by a LinkedList). PHP doesn't have this built in, so we would have to build our own.

Absent that, we could just use a regular array, inserting items in a circular manner once we reach $limit, overriding earlier indices as we go. This requires keeping track of the index ourselves, both for inserts and for the later yields.

At the time that I wrote the original implementation, I didn't feel like the juice is worth the squeeze. But if you want to tackle this, and @taylorotwell thinks it's worth it, go for it 👍

I'm curious. Do you actually have a use-case for this? Are you trying to use a negative take on a lazy collection in a real project? ↩

taylorotwell · 2023-09-13T16:40:07Z

This definitely adds a bit more complexity - what kind of performance / memory improvements are we looking at?

JosephSilber · 2023-09-13T16:59:35Z

@fuwasegu Here's the concept for how to use a circular array (not tested):

return new static(function () use ($limit) {
    $limit = abs($limit);
    $queue = [];
    $index = null;

    foreach ($this->getIterator() as $key => $value) {
        if ($index === null && count($queue) < $limit) {
            $queue[] = [$key, $value];
        }

        $index = $index === null || $index === $limit - 1 ? 0 : $index + 1;

        $queue[$index] = [$key, $value];
    }

    $yield = $index === null || $index === $limit - 1 ? 0 : $index + 1;
    $count = count($queue);

    for (; $yield < $count; $yield++) {
        yield $queue[$yield][0] => $queue[$yield][1];
    }

    if ($index !== null && $index !== $limit - 1) {
        for ($yield = 0; $yield < $index; $yield++) {
            yield $queue[$yield][0] => $queue[$yield][1];
        }
    }
});

A LimitIterator could potentially be used to simplify those 2 for loops.

rodrigopedra · 2023-09-14T01:21:04Z

@JosephSilber , pardon my ignorance, I really appreciate the work you've done with LazyCollection, and many other contributions.

Following the discussion, as \SplQueue is backed by a double-linked list, couldn't we be using it instead?

use Illuminate\Support\Facades\Artisan;
use Illuminate\Support\LazyCollection;

Artisan::command('local:test {size}', function () {
    $factory = function (): \Generator {
        $size = $this->argument('size');

        for ($index = 0; $index < $size; $index++) {
            yield $index => \number_format($index + 1);
        }
    };

    $collection = new class($factory) extends LazyCollection {
        public function take($limit)
        {
            if ($limit >= 0) {
                return parent::take($limit);
            }

            return new static(function () use ($limit) {
                $queue = new \SplQueue();
                $limit = -1 * $limit;

                foreach ($this as $key => $value) {
                    $queue->enqueue([$key, $value]);

                    if ($queue->count() > $limit) {
                        $queue->dequeue();
                    }
                }

                foreach ($queue as [$key, $value]) {
                    yield $key => $value;
                }
            });
        }
    };

    \dump(
        $collection->take(-3)->all(),
        \memory_get_usage(true),
        \microtime(true) - \LARAVEL_START,
    );
});

I ran the command above with increasing size values, execution time exploded with 100,000,000 elements, but memory consumption kept the same on all runs.

$ php artisan local:test 100
array:3 [ // routes/local.php:52
  97 => "98"
  98 => "99"
  99 => "100"
]
25165824 // routes/local.php:52
0.074693918228149 // routes/local.php:52

$ php artisan local:test 10000
array:3 [ // routes/local.php:52
  9997 => "9,998"
  9998 => "9,999"
  9999 => "10,000"
]
25165824 // routes/local.php:52
0.077880859375 // routes/local.php:52

$ php artisan local:test 1000000
array:3 [ // routes/local.php:52
  999997 => "999,998"
  999998 => "999,999"
  999999 => "1,000,000"
]
25165824 // routes/local.php:52
0.3977530002594 // routes/local.php:52

$ php artisan local:test 100000000
array:3 [ // routes/local.php:52
  99999997 => "99,999,998"
  99999998 => "99,999,999"
  99999999 => "100,000,000"
]
25165824 // routes/local.php:52
34.783122062683 // routes/local.php:52

rodrigopedra · 2023-09-14T01:34:34Z

By the way, about real world usage, I worked on a project that needed to import the last lines from a fixed-width file, where each line could have a different layout based on its first characters.

The last 3 or 5 lines (can't remember now) mattered for the use case, as they have some summary data and those files were huge.

I ended up using the approach above, this is why I remembered.

It is indeed a very niche use case (I hope never again to encounter such kind of file format!)

If this solution fits the balance between feature set and maintenance, maybe it could be a nice addition.

fuwasegu · 2023-09-14T03:47:14Z

Thank you for the various discussions and ideas you've provided! I've found them incredibly enlightening and am grateful for the learning opportunity.

Is there a use-case for taking elements from the end of a LazyCollection?

While I haven't personally encountered such a use-case, I became curious about the asymmetry in behavior for positive and negative $limit values (one being Lazy and the other Eager) when I was reviewing the LazyCollection code during my regular "Laravel Source Code Reading" livestream. Hence, the PR.

I also want to thank you for pointing out the inefficiency of array_shift in this context; it's something I had not realized. I was thrilled to see the ring buffer proposed as a solution! It's a data structure I'd only learned about in college and hadn't found a practical use for until now. It's an incredibly useful solution for this situation.

On the other hand, the implementation proposed by @JosephSilber was a bit difficult for me to grasp. With that said, I'd like to propose the following implementation:

public function take($limit)
{
    if ($limit < 0) {
        return new static(function () use ($limit) {
            $limit = abs($limit);
            $ringBuffer = [];
            $position = 0;

            foreach ($this as $key => $value) {
                $ringBuffer[$position] = [$key, $value];
                $position = ($position + 1) % $limit;
            }
            
            for ($i = 0; $i < $limit; $i++) {
                if (isset($ringBuffer[($position + $i) % $limit])) {
                    [$key, $value] = $ringBuffer[($position + $i) % $limit];
                    yield $key => $value;
                }
            }
        });
    }

    return parent::take($limit);
}

I've tried to determine the position of the current node using division in this new implementation.

Additionally, I ran benchmarks for the following patterns using the code that @rodrigopedra provided:

Using array_shift() ... Pattern A
Using a ring buffer ... Pattern B
Using SplQueue ... Pattern C
I measured the performance in the same manner for all three patterns.

Pattern A

$ php artisan take:with-array-shift 100000   
array:3 [ // routes/console.php:43
  99997 => "99,998"
  99998 => "99,999"
  99999 => "100,000"
]
20971520 // routes/console.php:43
0.15705704689026 // routes/console.php:43

$ php artisan take:with-array-shift  10000000 
array:3 [ // routes/console.php:43
  9999997 => "9,999,998"
  9999998 => "9,999,999"
  9999999 => "10,000,000"
]
20971520 // routes/console.php:43
3.0328080654144 // routes/console.php:43

$ php artisan take:with-array-shift  100000000
array:3 [ // routes/console.php:43
  99999997 => "99,999,998"
  99999998 => "99,999,999"
  99999999 => "100,000,000"
]
20971520 // routes/console.php:43
30.772088050842 // routes/console.php:43

Pattern B

$ php artisan take:with-ring-buffer 100000   
array:3 [ // routes/console.php:86
  99997 => "99,998"
  99998 => "99,999"
  99999 => "100,000"
]
20971520 // routes/console.php:86
0.091989994049072 // routes/console.php:86

$ php artisan take:with-ring-buffer 10000000 
array:3 [ // routes/console.php:86
  9999997 => "9,999,998"
  9999998 => "9,999,999"
  9999999 => "10,000,000"
]
20971520 // routes/console.php:86
2.7668669223785 // routes/console.php:86

$ php artisan take:with-ring-buffer 100000000
array:3 [ // routes/console.php:102
  99999997 => "99,999,998"
  99999998 => "99,999,999"
  99999999 => "100,000,000"
]
20971520 // routes/console.php:102
27.831671953201 // routes/console.php:102

Pattern C

$ php artisan take:with-spl-queue 100000   
array:3 [ // routes/console.php:128
  99997 => "99,998"
  99998 => "99,999"
  99999 => "100,000"
]
20971520 // routes/console.php:128
0.11904811859131 // routes/console.php:128

$ php artisan take:with-spl-queue 10000000   
array:3 [ // routes/console.php:128
  9999997 => "9,999,998"
  9999998 => "9,999,999"
  9999999 => "10,000,000"
]
20971520 // routes/console.php:128
3.190887928009 // routes/console.php:128

$ php artisan take:with-spl-queue 100000000  
array:3 [ // routes/console.php:144
  99999997 => "99,999,998"
  99999998 => "99,999,999"
  99999999 => "100,000,000"
]
20971520 // routes/console.php:144
33.102006912231 // routes/console.php:144

Yeah, array_shift is definitely slow, while the ring buffer shows great performance!

JosephSilber · 2023-09-14T16:25:30Z

@rodrigopedra Correct. Dequeuing manually is in essence a fixed-size queue.

Some of these SPL classes are quite slow. I was gonna suggest benchmarking it, but I see @fuwasegu already did 👍

@fuwasegu About your benchmarks: thanks for running them and sharing your findings 🙏

array_shift really gets worse the bigger the array is, so the numbers you posted don't fully reflect the differences. They're probably way more divergent with a higher $limit.

Could you rerun your benchmarks with this?

$collection->take(-floor($this->argument('size') / 2))->all();

Remember not to dump that result 😄

I was thrilled to see the ring buffer proposed as a solution

I don't think it's technically a ring buffer, as that would generally not overwrite earlier values. It would either wait, or throw an out of bounds exception. I'm not sure whether our implementation quantifies as a ring buffer. But yeah, it's a very similar concept.

On the other hand, the implementation proposed by @JosephSilber was a bit difficult for me to grasp.

Agreed. I was just quickly throwing it together, and trying to micro-optimize it so there's absolutely no extra work within the loops. But that was a mistake. Your code is way simpler and easier to read. I approve 👌

tests/Support/SupportLazyCollectionTest.php

rodrigopedra · 2023-09-14T17:03:52Z

@JosephSilber thanks for the response, I guess the circular queue implementation from @fuwasegu is the way to go.

I benchmarked a slightly modified version from @fuwasegu implementation and used your suggestion to take half of the elements from the end.

Time wise it still performs similarly, memory wise of course it explodes as we are keeping half of the elements.

As a sidenote, I modified the implementation a bit, so I could understand it better.

Artisan::command('local:ring {size}', function () {
    $factory = function (): \Generator {
        $size = $this->argument('size');

        for ($index = 0; $index < $size; $index++) {
            yield $index => \number_format($index + 1);
        }
    };

    $collection = new class($factory) extends LazyCollection {
        public function take($limit)
        {
            if ($limit >= 0) {
                return parent::take($limit);
            }

            return new static(function () use ($limit) {
                $limit = -1 * $limit;

                $queue = [];
                $head = 0;

                foreach ($this as $key => $value) {
                    if (\count($queue) < $limit) {
                        $queue[] = [$key, $value];
                    } else {
                        $head = ($head + 1) % $limit;
                        $queue[($head ?: $limit) - 1] = [$key, $value];
                    }
                }

                for ($index = 0; $index < \count($queue); $index++, $head = ($head + 1) % $limit) {
                    [$key, $value] = $queue[$head];
                    yield $key => $value;
                }
            });
        }
    };

    $collection->take(-floor($this->argument('size') / 2))->all();

    \dump(
        \memory_get_usage(true),
        \microtime(true) - \LARAVEL_START,
    );
});

Results for the circular queue above:

$ php artisan local:ring 100
25165824 // routes/local.php:57
0.07240104675293 // routes/local.php:57

$ php artisan local:ring 10000
25165824 // routes/local.php:57
0.075173139572144 // routes/local.php:57

$ php artisan local:ring 1000000
150994944 // routes/local.php:57
0.42445397377014 // routes/local.php:57

$ php artisan local:ring 100000000
PHP Fatal error:  Allowed memory size of 1073741824 bytes exhausted (tried to allocate 4096 bytes) in /home/rodrigo/code/tmp/routes/local.php on line 40
PHP Fatal error:  Allowed memory size of 1073741824 bytes exhausted (tried to allocate 8192 bytes) in /home/rodrigo/code/tmp/vendor/symfony/error-handler/Error/FatalError.php on line 48

As a comparison, these are the results for previous \SqlQueue implementation, taking half of the elements as suggested.

$ php artisan local:spl 100
25165824 // routes/local.php:100
0.072839975357056 // routes/local.php:100

$ php artisan local:spl 10000
25165824 // routes/local.php:100
0.077234029769897 // routes/local.php:100

$ php artisan local:spl 1000000
165675008 // routes/local.php:100
0.44696998596191 // routes/local.php:100

$ php artisan local:spl 100000000
PHP Fatal error:  Allowed memory size of 1073741824 bytes exhausted (tried to allocate 4096 bytes) in /home/rodrigo/code/tmp/routes/local.php on line 68
PHP Fatal error:  Allowed memory size of 1073741824 bytes exhausted (tried to allocate 8192 bytes) in /home/rodrigo/code/tmp/vendor/symfony/error-handler/Error/FatalError.php on line 48

I did not benchmark the \array_shift() implementation.

This reverts commit 601eb3b.

src/Illuminate/Collections/LazyCollection.php

Co-authored-by: Joseph Silber <[email protected]>

fuwasegu · 2023-09-18T14:29:16Z

@taylorotwell
Finally, you are the one who needs to confirm!

taylorotwell · 2023-09-18T18:32:36Z

Thank you!

fuwasegu · 2023-09-19T01:27:26Z

Thank you all for your comments and reviews!
It has been a great learning experience for me!

refactor: stop using paththru in take method

f9fea87

fuwasegu force-pushed the fix/lazy-collection-take-memory-saving branch from b7bd475 to f9fea87 Compare September 13, 2023 14:23

fuwasegu changed the title ~~[10.x] Refactored LazyCollection::take() to save memory.~~ [10.x] Refactored LazyCollection::take() to save memory Sep 13, 2023

taylorotwell marked this pull request as draft September 13, 2023 14:37

JosephSilber reviewed Sep 13, 2023

View reviewed changes

taylorotwell marked this pull request as ready for review September 13, 2023 16:40

fuwasegu added 2 commits September 14, 2023 16:03

refactor: use ring buffer

bc8df43

test: add negative limit test

601eb3b

fuwasegu requested a review from JosephSilber September 14, 2023 08:01

JosephSilber reviewed Sep 14, 2023

View reviewed changes

tests/Support/SupportLazyCollectionTest.php Outdated Show resolved Hide resolved

Revert "test: add negative limit test"

a070550

This reverts commit 601eb3b.

fuwasegu requested a review from JosephSilber September 15, 2023 08:53

JosephSilber approved these changes Sep 15, 2023

View reviewed changes

JosephSilber reviewed Sep 18, 2023

View reviewed changes

src/Illuminate/Collections/LazyCollection.php Outdated Show resolved Hide resolved

Update src/Illuminate/Collections/LazyCollection.php

4cb69c1

Co-authored-by: Joseph Silber <[email protected]>

fuwasegu requested a review from JosephSilber September 18, 2023 14:28

JosephSilber approved these changes Sep 18, 2023

View reviewed changes

taylorotwell merged commit 9ac653d into laravel:10.x Sep 18, 2023
19 checks passed

fuwasegu deleted the fix/lazy-collection-take-memory-saving branch September 19, 2023 01:22

fuwasegu mentioned this pull request Sep 25, 2023

[10.x] Refactored LazyCollection::except() to save memory #48535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[10.x] Refactored LazyCollection::take() to save memory #48382

[10.x] Refactored LazyCollection::take() to save memory #48382

fuwasegu commented Sep 13, 2023 •

edited

Loading

taylorotwell commented Sep 13, 2023

JosephSilber left a comment •

edited

Loading

taylorotwell commented Sep 13, 2023

JosephSilber commented Sep 13, 2023 •

edited

Loading

rodrigopedra commented Sep 14, 2023

rodrigopedra commented Sep 14, 2023

fuwasegu commented Sep 14, 2023

JosephSilber commented Sep 14, 2023

rodrigopedra commented Sep 14, 2023

fuwasegu commented Sep 18, 2023

taylorotwell commented Sep 18, 2023

fuwasegu commented Sep 19, 2023

[10.x] Refactored LazyCollection::take() to save memory #48382

[10.x] Refactored LazyCollection::take() to save memory #48382

Conversation

fuwasegu commented Sep 13, 2023 • edited Loading

Introduction

Main Topic

Implementation

About Tests

taylorotwell commented Sep 13, 2023

JosephSilber left a comment • edited Loading

Choose a reason for hiding this comment

Footnotes

taylorotwell commented Sep 13, 2023

JosephSilber commented Sep 13, 2023 • edited Loading

rodrigopedra commented Sep 14, 2023

rodrigopedra commented Sep 14, 2023

fuwasegu commented Sep 14, 2023

JosephSilber commented Sep 14, 2023

rodrigopedra commented Sep 14, 2023

fuwasegu commented Sep 18, 2023

taylorotwell commented Sep 18, 2023

fuwasegu commented Sep 19, 2023

fuwasegu commented Sep 13, 2023 •

edited

Loading

JosephSilber left a comment •

edited

Loading

JosephSilber commented Sep 13, 2023 •

edited

Loading